Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for reverse telnet #1920

Merged
merged 4 commits into from
Oct 9, 2019
Merged

Added support for reverse telnet #1920

merged 4 commits into from
Oct 9, 2019

Conversation

Niek
Copy link
Contributor

@Niek Niek commented Sep 23, 2019

Not sure if this is a useful feature to add, but I could see it being useful for some people. This basically adds the ability to telnet to an Espurna device that is NATed/firewalled. After sending the host:ip of a publicly reachable ncat listener to the MQTT topic telnet_reverse, the device opens a telnet connection.

Note: untested on Async telnet server, but should work. There's one more issue that the first command is ignored, probably easiest is to move _telnetFirst = true to _telnetLoop / _telnetNewClient and set it to false in _telnetReverse?

@mcspr
Copy link
Collaborator

mcspr commented Sep 23, 2019

It could be useful, but something to consider:

Telnet is a bit tricky, because we are not exactly doing real telnet and would need to look at the network stream to check what is broken there (line-endings, some control codes or something else)
telnet.netkit works on the first try, but netcat or busybox's telnet do not

@Niek
Copy link
Contributor Author

Niek commented Sep 23, 2019

I didn't see #1505, but MQTT seems to be less suited for this. First of all I don't think it's very useful to send full terminal output over MQTT. Secondly, this is especially useful when debugging connection issues like MQTT/OTA/etc.

I tested with ncat only, netcat doesn't seem to work indeed (and I don't see an option to use busybox telnet in listen mode). It'll need some further testing for sure.

@mcspr
Copy link
Collaborator

mcspr commented Sep 23, 2019

Why I mentioned action, I wonder if that can be reused instead of adding another topic. Question is, should it be just another teminal input or something with more extended syntax. It is not utilized in any way right now besides a simple reboot.
-t some/action -m "telnet.reverse value"
-t some/action -m '{"cmd": "telnet.reverse value"}
-t some/action -m '{"action":"command","data":{"command":"set","arguments":"key value"}' (something like ws does already)
-t some/action/cmd -m 'reset'

Input could be useful, but logging needs to support module / severity filtering and perhaps target filtering (current mqttSend logs things, which logs things etc...)
What it does is makes a single topic for multiple devices, like MQTT relay grouping does

@Niek
Copy link
Contributor Author

Niek commented Sep 24, 2019

Ah, you were talking about the MQTT topic only. Yes, I think it does makes sense - if the action command is extended some topics can be removed (OTA, relay, and this one come to mind). I think the plain one (not JSON) is the easiest approach.

Coming back to the PR: I have changed the telnetFirst code now, it works fine in both netcat and ncat now.

@mcspr
Copy link
Collaborator

mcspr commented Sep 24, 2019

Will test that asap.
Note the first mention of this is from here: d69d6bf
Does this work with real telnet clients thought? And stuff like C-d still resets the connection?

Do you mean pulse action for relays? I do remember that this was initially proposed as root/relay/N/pulse/set (#902), but Xose had reverted it into the root/pulse/N/set instead. There's overlapping dependency on HTTP API.
I guess this can be merged as-is, but I'd rather think a bit about how actions can work here instead of adding one more top level topic.

@Niek
Copy link
Contributor Author

Niek commented Sep 24, 2019

Will test that asap.
Note the first mention of this is from here: d69d6bf
Does this work with real telnet clients thought? And stuff like C-d still resets the connection?

With reverse telnet, Espurna is basically the client and the netcat/nc/whatever listening socket is the server. But yes, everything works as expected like Ctrl+C, quit command, etc.

The firstline stuff is in there because this is part of the telnet negotiation (should probably be handled differently than naively expecting it to be the first packet). Because the socket that we connect to is not an actual telnet client, there's no need to skip the first data packet.

Do you mean pulse action for relays? I do remember that this was initially proposed as root/relay/N/pulse/set (#902), but Xose had reverted it into the root/pulse/N/set instead. There's overlapping dependency on HTTP API.
I guess this can be merged as-is, but I'd rather think a bit about how actions can work here instead of adding one more top level topic.

If the action topic would be extended, I think that every command that can be done with terminal commands should be done with this topic, right?

@mcspr
Copy link
Collaborator

mcspr commented Sep 25, 2019

What I meant is how that worked with existing functionality. I do see that is ok, so no problems there.

Thanks for the link! I did end up reading telnet source anyway, but the gist of it that we should probably ignore sequences like 255 something something and 255 250 ...multiple bytes... 240 instead of ignoring the first packet.

code/espurna/telnet.ino Outdated Show resolved Hide resolved
code/espurna/telnet.ino Outdated Show resolved Hide resolved
@Niek
Copy link
Contributor Author

Niek commented Sep 26, 2019

What I meant is how that worked with existing functionality. I do see that is ok, so no problems there.

Thanks for the link! I did end up reading telnet source anyway, but the gist of it that we should probably ignore sequences like 255 something something and 255 250 ...multiple bytes... 240 instead of ignoring the first packet.

I think it can be as easy as changing the code to this, right (in _telnetData())?

if (len >= 2) {
  // Ctrl+C
  if ((p[0] == 0xFF) && (p[1] == 0xEC)) {
      _telnetDisconnect(clientId);
      return;
  // Negotiation stuff
  } else if (p[0] == 0xFF) && (p[1] != 0xFF)) {
    return;
  }
}

Then the firstLine stuff can be removed completely.

@mcspr
Copy link
Collaborator

mcspr commented Sep 26, 2019

Yep, something like that. As a separate pr though.
Second condition would only handle data at the beginning. As I understood it telnet client would replace 0xff with 0xff 0xff anywhere in the data stream, but in what case user would send it?

i.e. maybe like this? i wonder if that would skip data when using some scripting, when telnet client sends negotiation data and text in the same packet

#define TELNET_IAC  0xFF
#define TELNET_XEOF 0xEC

...
    if (len >= 2) && (p[0] == TELNET_IAC) {
        // C-d is sent as two bytes (sometimes repeating)
        if (p[1] == TELNET_XEOF) {
            _telnetDisconnect(clientId);
        }
        return;
    }

edit: fatfingers...

mcspr pushed a commit that referenced this pull request Sep 30, 2019
* Get rid of _telnetFirst (assumption that first line is always telnet renegotiation)

Discussed in: #1920 (comment)
@mcspr mcspr merged commit f3b3556 into xoseperez:dev Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants